Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NXcomponent as a parent base class #1525

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

lukaspie
Copy link
Contributor

@lukaspie lukaspie commented Dec 19, 2024

Disclaimer: This is a draft and not meant to be voted on yet. The idea here is to perform an exploration/survey of the consequences of what having an NXcomponent base class would mean.

This implements the discussion items in #1519 (comment).

NXcomponent is meant as a parent base class that many base classes describing devices/instrument components should derive from. As requested in the comment above, this is meant as a survey of base classes to identify those which should and those which should not derive from NXcomponent.

These are the base classes that are now inheriting from NXcomponent:
in base_classes:

  • NXaperture
  • NXattenuator
  • NXbeam_stop
  • NXbending_magnet
  • NXcapillary
  • NXcollimator
  • NXcrystal
  • NXdetector
  • NXdisk_chopper
  • NXelectron_detector (through NXdetector)
  • NXfermi_chopper
  • NXfilter
  • NXflipper
  • NXfresnel_zone_plate
  • NXgrating
  • NXguide
  • NXinsertion_device
  • NXmirror
  • NXmoderator
  • NXmonitor
  • NXmonochromator
  • NXpinhole
  • NXpolarizer
  • NXpositioner
  • NXsample
  • NXsample_component
  • NXsensor
  • NXslit
  • NXvelocity_selector
  • NXxraylens

in contributed_definitions:

  • NXaperture_em
  • NXbeam_splitter
  • NXchamber
  • NXcollectioncolumn
  • NXcontainer
  • NXcorrector_cs
  • NXdeflector
  • NXebeam_column
  • NXelectronanalyser
  • NXenergydispersion
  • NXibeam_column
  • NXlens_em
  • NXlens_opt
  • NXmagnetic_kicker
  • NXmanipulator
  • NXpid
  • NXpolarizer_opt
  • NXpulser_apm
  • NXpump
  • NXquadrupole_magnet
  • NXreflectron
  • NXscanbox_em
  • NXseparator
  • NXsolenoid_magnet
  • NXspin_rotator
  • NXspindispersion
  • NXstage_lab
  • NXwaveplate

In addition, the default attribute is added to NXobject so that it doesn't have to be repeated in every class. As a consequence, it can be removed This affects almost all of the classes above, but additionally the default attribute is removed from these classes (which are still inheriting from NXobject, not from NXcomponent):

  • NXbeam
  • NXcite
  • NXdetector_group
  • NXdetector_module
  • NXevent_data
  • NXgeometry
  • NXinstrument
  • NXlog
  • NXnote
  • NXoff_geometry
  • NXorientation
  • NXparameters
  • NXprocess
  • NXreflections
  • NXshape
  • NXtransformations
  • NXtranslation
  • NXuser

further changes:

  • NXcylindrical_geometry: rename "beamline component" to "component" in docs
  • NXoff_geometry: rename "beamline component" to "component" in docs

Open questions:

  • Should any of these also inherit from NXcomponent?
    • NXbeam
    • NXdetector_module
    • NXinstrument

@lukaspie
Copy link
Contributor Author

lukaspie commented Jan 16, 2025

Quick comment that I missed to mention in today's telco: are there any tools that would expect these classes to extend NXobject and not NXcomponent? For example, conditional checks like if group.attribute(NX_class) == "NXobject".

We had a similar problem before when we tried to subclass NXdata (e.g. to NXdata_mpes) that some of the visualization tools (H5web in particular) are only checking for NXdata and not for subclassed versions, so the default visualization breaks.

Since base class inheritance is now supported, such checks will anyway break on a smaller scale, e.g. for NXelectron_detector extending NXdetector (already part of the standard), so maybe it would be even good to do this once for many of the fundamental classes (like here) so that such errors are seen in most places and can be corrected immediately.

@rayosborn
Copy link
Contributor

The question of whether there are tools to implement inheritance is impossible to answer. I can modify the nexusformat package to explicitly include this inheritance, since all the base classes are really classes (i.e., not just objects with an NX_class attribute). However, inheritance is meaningless if someone is using, say, h5py or H5web directly, since HDF5 groups have no class. The NXvalidate package explicitly extend the NXobject class and will, I believe, automatically extend the NXcomponent class once it is approved, but again that is up to the package developer.

That is one reason I have argued in the past for a package like NeXpy to be officially supported or at least recommended, i.e., so that there is a generic NeXus viewer that is guaranteed to follow changes in the standard, but I lost that battle years ago.

@lukaspie
Copy link
Contributor Author

lukaspie commented Jan 27, 2025

As suggested in #1408 (comment), here is a flow diagram with the new inheritance (not containing changes to the contributed definitions).

NXcomponent_inheritance

@lukaspie
Copy link
Contributor Author

I also added inputs and outputs to NXcomponent: lists of other components (NX_CHAR, rank of 0 or 1 (single or multiple)). This is to be used to chain components and/or beams together, as discussed originally in #1408.

@lukaspie
Copy link
Contributor Author

Removed NXcircuit from NXcomponent as discussed in today's Telco.

@phyy-nx
Copy link
Contributor

phyy-nx commented Jan 27, 2025

Hi all, as discussed in the Telco, we can now move this PR to an online vote. NIAC committee members please vote on this PR using emojis on this comment. 👍 for yes, 👎 for no, anything else (for example 👀) to abstain. We need 14 votes to hit quorum so please review and vote!

</doc>
<field name="applied" type="NX_BOOLEAN">
<doc>
Was the component used?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is somewhat vague: what does "used" mean, in this context?

extending ``NXcomponent``) or ``NXbeam`` that act as input(s) to this
component.

Each input should point to the path of the group acting as input.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description leaves me with a number of questions:

Where is the format for a path defined? Can the path be absolute, can it be relative?

How are the paths within the list concatenated into a single string? (space-separated list? comma-separated list? colon-separated list?) If paths have separator character, is white space allowed (or required) around separator character?

Is there an escaping mechanism? One of the paths in the list might contain a path-separator character. Without escaping, this could lead to ambiguity.

Is there any significance placed on the order of paths within this list?

The approach of serialising paths seems to be reimplementing HDF5's symlink feature. Why not just use symlinks, instead?

extending ``NXcomponent``) or ``NXbeam`` that act as output(s) of this
component.

For more information, see :ref:`inputs &lt;/NXcomponent/inputs-field&gt;`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description leaves me with the same questions as with the inputs field.

-->
<definition xmlns="http://definition.nexusformat.org/nxdl/3.1" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" category="base" type="group" name="NXcomponent" extends="NXobject" xsi:schemaLocation="http://definition.nexusformat.org/nxdl/3.1 ../nxdl.xsd">
<doc>
Base class for components of an instrument - real ones or simulated ones.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest also mentioning that a NXcomponent has some well-defined position in space.

@paulmillar
Copy link

Just to be clear: I really like the idea behind this PR and would like to see this go into NeXus.

However, I'd like the descriptions to be improved; in particular, the comments for the "inputs" field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants